[HDX-5079] Render formulas in the composed metric query - #2908
Conversation
wrn14897
added
the
ai-generated
🦋 Changeset detectedLatest commit: d06dbba The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThe PR adds SQL compilation and composed-query rendering for metric formulas, including single-series routing and raw-SQL template conversion.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported malformed alias quoting is fixed across all composed projection paths.
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/core/formula.ts | Adds a validated-AST-to-SQL compiler with coalesced operands and guarded division. |
| packages/common-utils/src/core/renderChartConfig.ts | Adds formula-aware composed projections and correctly fixes quoted aliases across formula, operand, and ratio columns. |
| packages/common-utils/src/core/builderToRawSql.ts | Enables composed multi-series, ratio, and formula metric queries in generated raw-SQL templates. |
| packages/common-utils/src/tests/queryChartConfig.int.test.ts | Covers formula execution, missing-data behavior, mixed metric types, grouping, metadata ordering, and supported display shapes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[Metric chart config] --> Validate[Validate formula AST]
Validate --> Branches[Render one query branch per operand]
Branches --> Union[UNION ALL]
Union --> Pivot[Pivot values by series index]
Pivot --> Compile[Compile formula over pivot expressions]
Compile --> Project[Project operands and formula aliases]
Project --> ClickHouse[Execute composed ClickHouse query]
Reviews (6): Last reviewed commit: "Merge branch 'main' into warren/HDX-5079..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 299 passed • 1 skipped • 1081s
Tests ran across 4 shards in parallel. |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Additional context: touches the query rendering engine lightly (142 lines, under the 150-line bar for Tier 4) Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Deep ReviewRendering metric formulas ( ✅ No critical issues found. No P0/P1 defects; the diff is safe to merge. One test gap and a couple of small maintainability nits below. 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (8 dispatched): correctness, security, adversarial, testing, maintainability, kieran-typescript, api-contract, learnings-researcher. Graded findings reflect the testing and maintainability reviewers plus direct orchestrator analysis of the correctness, security, and API-contract dimensions (all clean); the learnings researcher found no prior Testing gaps:
|
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM, with a comment that could be a followup if it makes sense.
| if ( | ||
| isMetric && | ||
| Array.isArray(config.select) && | ||
| (config.select.length > 1 || (config.formulas?.length ?? 0) > 0) | ||
| ) { | ||
| return { | ||
| isError: true, | ||
| error: 'Multi-series metric charts cannot be auto-converted to SQL.', | ||
| error: | ||
| (config.formulas?.length ?? 0) > 0 | ||
| ? 'Metric charts with formulas cannot be auto-converted to SQL.' | ||
| : 'Multi-series metric charts cannot be auto-converted to SQL.', |
There was a problem hiding this comment.
This was a limit originally because multi-series metrics couldn't be represented as a single query. Now they can, and so can formulas + ratios. Is there a reason to continue blocking this case?
There was a problem hiding this comment.
Good point. I believe we should be able to show the query now. Let me revisit the code here
There was a problem hiding this comment.
No technical reason anymore — you're right that the composed shape lifted the original constraint. I checked what was left:
- The per-series branches inherit
isRenderingRawSqlTemplate, so each branch already renders with$__sourceTable(<metricType>)and the time/interval macros, same as the single-series conversion. replaceMacrosresolves multiple$__sourceTable(type)occurrences with per-occurrence args,$__filterslands once per branch source CTE (never in the outer pivot), the hoistedSETTINGSare literal, and the outer pivot/formula projection binds no params.
The only reason it was still blocked was scope — HDX-5077 listed builderToRawSql multi-series support as a follow-up, and this gate just extended that to formulas with a clearer message.
Lifted in d63af10: multi-series, ratio, and formula metric charts now convert to a macro-based template (with snapshot tests, $__filters-placement coverage, and a replaceMacros round-trip). Non-time-series metric charts keep the existing restriction.
Compile the validated formula AST (HDX-5078) into the final SELECT projection over the pivoted per-series columns produced by the composed multi-series metric query (HDX-5077). - compileFormulaAst: letter refs resolve to the pivot expressions with ratio-consistent semantics (missing operand -> 0, division by zero or missing denominator -> NULL, rendered as a gap) - Formula columns append after the operand value columns (select order), preserving the useChartNumberFormats positional meta contract; showOperandSeries: false emits only the formula column(s) - Single-series metric charts with a formula route through the composed path; formulas take precedence over seriesReturnType: 'ratio' - builderToRawSql rejects formula configs (same limitation as multi-series metric charts)
A formula or series alias containing a double quote terminated the double-quoted identifier early (AS "bad"name") and failed the query with a ClickHouse syntax error. Escape by doubling the quote at SQL-emission time across all composed-projection sites (formula, operand, and ratio columns); collision dedup and result meta keep the raw name. Addresses greptile P1 review comment.
8d9e37e to
41b2354
Compare
Deep Review✅ No critical issues found. This is a self-contained SQL-rendering change with heavy unit + integration test coverage. The two highest-risk surfaces were verified clean:
🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers: synthesized from direct diff analysis across correctness, security/SQL-injection, testing, and maintainability lenses. (The dispatched persona sub-agents had not returned findings at synthesis time; findings above are verified against the diff and surrounding code.) Testing gaps: formula numeric literals that render in exponential notation are not covered by an explicit assertion, though the resulting SQL is valid. |
The multi-series gate in renderBuilderConfigAsSqlTemplate predated the composed single-query renderer: each per-series branch now emits its own $__sourceTable(<metricType>) and time macros, $__filters lands once per branch source CTE, the hoisted SETTINGS are literal, and the outer pivot binds no params — so multi-series, ratio, and formula metric charts convert to a raw-SQL template like any other metric chart. Non-time-series metric charts keep the existing restriction. Addresses review feedback on the formula gate.
3 participants
Summary
Renders metric formulas (HDX-5078's
formulasconfig) in the composed multi-series metric query (HDX-5077), so a derived series likeA / (A + B + C) * 100is computed by ClickHouse as part of the single composed query instead of not rendering at all.compileFormulaAst(core/formula.ts): compiles the validated letter-ref AST into a SQL expression over per-series value expressions. Never splices user text into SQL — only the parsed/validated AST is walked.coalesce(<pivot>, 0)(a missing operand counts as 0, so a zero-error group reads 0%, not N/A), and every division denominator is wrapped innullif(..., 0)(zero or missing denominator → NULL → rendered gap, never 0 or an error).renderMultiSeriesMetricChartConfig): operand value columns first in select order, then formula columns in formulas order, ahead of the group/bucket passthrough columns — preserving theuseChartNumberFormatspositional meta contract.showOperandSeries: falsedrops the operand columns so only the formula column(s) are returned.A * 100) now routes through the composed path (single-branch union pivot). Per-series branches stripformulasto avoid recursion.seriesReturnType: 'ratio'(the two are mutually exclusive in the editor; the renderer stays deterministic on a hand-built config carrying both).builderToRawSql: formula configs are rejected from "convert to SQL" with a clear message (same limitation as multi-series metric charts).Alerts on formula tiles work with no changes since they query through
queryChartConfig→renderChartConfig.Testing
make ci-lint,make ci-unitpass.compileFormulaAst(precedence, nested divisions, unary minus, HDX-4938 example) and SQL snapshot tests for the formula projection (grouped, hidden operands, single-series routing, formula-vs-ratio precedence, alias collision suffixing, invalid-formula error).queryChartConfig.int.test.ts(all passing against the docker ClickHouse, with the HDX-5076/5077 regression baseline unchanged):A / (A + B + C) * 100seriesReturnType: 'ratio'test for drop-in parity)How to test on Vercel preview
N/A — query-rendering change in common-utils; the chart editor UI for formulas lands in HDX-5080.
References